Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: (x/bank) add spendable balances cmd #14045

Merged
merged 24 commits into from
Jan 5, 2023
Merged

feat: (x/bank) add spendable balances cmd #14045

merged 24 commits into from
Jan 5, 2023

Conversation

facundomedica
Copy link
Member

@facundomedica facundomedica commented Nov 28, 2022 •
edited
Loading

Description

This PR adds a spendable-balances to x/bank's queries.

Questions:

  1. Should we move this command under balances and activate it with a bool flag (--only-spendable)? This would invalidate the --denom flag. If we are ok with that, maybe it's cleaner that way.

Closes: #14030

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@facundomedica facundomedica requested a review from a team as a code owner November 28, 2022 14:42
@likhita-809
Copy link
Contributor

  1. Should we move this command under balances and activate it with a bool flag (--only-spendable)? This would invalidate the --denom flag. If we are ok with that, maybe it's cleaner that way.

I think spendableCoins uses gas more efficiently when compared to get the balance of an account multiple times after txs.
Maybe I'd prefer having it in a separate command, not to invalidate existing functionality of --denom flag usage.
cc @alexanderbez

@alexanderbez
Copy link
Contributor

  1. Should we move this command under balances and activate it with a bool flag (--only-spendable)? This would invalidate the --denom flag. If we are ok with that, maybe it's cleaner that way.

I think spendableCoins uses gas more efficiently when compared to get the balance of an account multiple times after txs. Maybe I'd prefer having it in a separate command, not to invalidate existing functionality of --denom flag usage. cc @alexanderbez

I like @facundomedica's proposal! Why exactly do you think having a --spendable-only flag is a bad idea?

@likhita-809
Copy link
Contributor

likhita-809 commented Nov 29, 2022 •
edited
Loading

  1. Should we move this command under balances and activate it with a bool flag (--only-spendable)? This would invalidate the --denom flag. If we are ok with that, maybe it's cleaner that way.

I think spendableCoins uses gas more efficiently when compared to get the balance of an account multiple times after txs. Maybe I'd prefer having it in a separate command, not to invalidate existing functionality of --denom flag usage. cc @alexanderbez

I like @facundomedica's proposal! Why exactly do you think having a --spendable-only flag is a bad idea?

I'm not opposed to it and don't really think its a bad idea, but just concerned if we could remove --denom's functionality and its not needed in future, but definitely agree that it makes balances cleaner.

@facundomedica
Copy link
Member Author

So in other words --denom will be ignored if --spendable-only is passed. Another option would be adding a new query that only gets the spendable amount of an specific denom to mimic what GetBalances is doing. Then both --denom and --spendable-only could be used together.

@likhita-809
Copy link
Contributor

So in other words --denom will be ignored if --spendable-only is passed. Another option would be adding a new query that only gets the spendable amount of an specific denom to mimic what GetBalances is doing. Then both --denom and --spendable-only could be used together.

I like this better

@alexanderbez
Copy link
Contributor

@facundomedica well if it's a new query for just spendable balances, then --spendable-only isn't needed anymore and we're back to where we started, no?

@facundomedica
Copy link
Member Author

Coming back to this after a while. I think we have to options:

  1. Modify the current GetBalancesCmd to take in --spendable-only. For this we would need to add a new query like SpendableBalance (And maybe modify SpendableBalances to AllSpendableBalances, but that would be breaking). To mimic Balance and AllBalances, and making possible to get a single denom by passing --denom.
  2. Leave this PR as is, returning a list of spendable balances in a separate command.

If we really want to be able to get a single denom at a time, we should go for 1, if not, I would just stick with 2.

@facundomedica
Copy link
Member Author

So now the spendable-balances accepts the --denom flag, so it works just like the balances cmd.

@alexanderbez
Copy link
Contributor

@facundomedica let's move this to draft mode until it's ready to review again pls.

@tac0turtle tac0turtle marked this pull request as draft December 27, 2022 12:59
@sonarcloud
Copy link

sonarcloud bot commented Jan 2, 2023

[Cosmos SDK] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
No Duplication information No Duplication information

@facundomedica facundomedica marked this pull request as ready for review January 2, 2023 15:15
likhita-809
likhita-809 approved these changes Jan 4, 2023
View reviewed changes
Copy link
Contributor

@likhita-809 likhita-809 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one comment, otherwise lgtm
can you also remove the gov expected_keeper_mocks from here and fix these failing tests ?

x/bank/client/cli/query_test.go Show resolved Hide resolved
tac0turtle
tac0turtle reviewed Jan 4, 2023
View reviewed changes
x/bank/keeper/grpc_query.go
@@ -47,10 +43,6 @@ func (k BaseKeeper) AllBalances(ctx context.Context, req *types.QueryAllBalances
return nil, status.Error(codes.InvalidArgument, "empty request")
}

if req.Address == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why these removals

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this condition is checked again using sdk.AccAddressFromBech32(req.Address) in L46 https://github.com/cosmos/cosmos-sdk/pull/14045/files#diff-c2a808829580789ad27e7f3393b3cd63b2f61ff6df06de938cd14dfd73d01e9fR46

tac0turtle
tac0turtle reviewed Jan 4, 2023
View reviewed changes
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lef tone question but looks good otherwise

@alexanderbez alexanderbez enabled auto-merge (squash) January 5, 2023 15:50
@alexanderbez
Copy link
Contributor

LGTM. Let's merge.

@alexanderbez alexanderbez merged commit 6ac0c36 into main Jan 5, 2023
@alexanderbez alexanderbez deleted the facu/spendable-balances-cmd branch January 5, 2023 19:58
@mergify mergify bot mentioned this pull request Jan 5, 2023
Merged
mergify bot pushed a commit that referenced this pull request Jan 5, 2023
@facundomedica @mergify
feat: (x/bank) add spendable balances cmd (#14045)
a993d24 
(cherry picked from commit 6ac0c36)

# Conflicts:
#	CHANGELOG.md
julienrbrt added a commit that referenced this pull request Jan 5, 2023
@mergify @facundomedica @julienrbrt
feat: (x/bank) add spendable balances cmd (backport #14045) (#14514)
b659774 
Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
@0xstepit 0xstepit mentioned this pull request Jul 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tac0turtle tac0turtle tac0turtle left review comments

@julienrbrt julienrbrt julienrbrt left review comments

@likhita-809 likhita-809 likhita-809 approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

Assignees

@JeancarloBarrios JeancarloBarrios

@samricotta samricotta

@likhita-809 likhita-809

Labels
backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release C:CLI C:x/bank C:x/gov
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Request to add spendable amount CLI query in bank module
7 participants
@facundomedica @likhita-809 @alexanderbez @julienrbrt @tac0turtle @JeancarloBarrios @samricotta